fix(catalog): publish cache assets without overwriting entries - #3739
Conversation
Fallow audit reportFound 8 findings. Details
Generated by fallow. |
|
Closing — this PR has merge conflicts and has been inactive. Thank you for the contribution! Please feel free to reopen with a rebased branch. |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Exact-head security/concurrency review at 6cde0fab99d2dd3f81c227932cf7e922f44684a4.
scripts/catalog-payload-assets.ts:217preserves cache hits—including dangling leaves—without requiringmkdir, staging, or write access.scripts/catalog-payload-assets.ts:221-229stages complete bytes in an atomically created private sibling and makeslinkSyncthe sole publication operation; onlyEEXISTis swallowed, so a competing regular file or live/dangling link is never replaced.scripts/catalog-payload-assets.ts:230-234keeps cleanup best-effort without masking the original write/link failure or turning a successful publication into an error.scripts/catalog/catalog-payload-assets.cache-race.test.ts:95-162covers both callers across cache reuse, competing entries, directory links, partial writes, publication failure, and cleanup failure. The two focused catalog race suites pass 44/44 locally..github/workflows/windows-render.yml:487-495resolves the rebase conflict correctly: the newer frame-staging test remains, and both catalog file-operation suites run in the Windows lane.
No blocking findings. The disclosed remaining pathname trust boundary for hostile concurrent ancestor swaps is real but not widened by this patch; the optional Fallow failure is the disclosed cyclomatic-8 helper plus inherited findings, with no suppression. Required Test and Windows checks are still pending at review time, so branch protection remains the merge gate.
Verdict: APPROVE
Reasoning: The implementation gives both hashed-cache writers one atomic, no-replace publication boundary while preserving existing hit, hash, URL, MIME, threshold, and count behavior; the adversarial tests exercise the relevant state transitions and cleanup paths.
— Magi
3 participants
Catalog asset references and large embedded data URIs checked for an existing hashed output and then wrote by pathname. A competing file or symlink could appear between those operations and be overwritten. Both callers now share cache publication that stages complete bytes in a private sibling directory, then atomically links them into place only if the destination is still absent. This addresses CodeQL #851 and #852.
Existing cache entries remain untouched, including dangling links, and cache hits require no staging or writable directory. Hashes, URLs, MIME handling, externalization thresholds and result counts are preserved. Cache misses reject a symlinked cache directory. Cleanup is best effort so it cannot mask a write/publication failure or turn a successful publication into an error. The output directory's ancestors remain trusted against hostile concurrent parent renames; cleanup failure may leave private staging residue. Directory-copy overwrite behavior is unchanged.
Validation: all 274 script/catalog tests (188 Node + 86 Vitest), script types, dependency builds, lint/format and signed hooks pass. Twenty new cases cover both callers: competing files/live or dangling links, existing dangling entries, external directory links, complete publication, cache reuse, partial-write cleanup and publication/cleanup failures. The competing-entry, dangling-link, directory-link and partial-write witnesses fail against main. The new suite joins the existing catalog source-read suite in the Windows lane.
Optional Fallow remains red locally: eight complexity findings, seven classified as inherited, and no duplication groups. The new cache helper has cyclomatic complexity 8; its branches enforce cache-hit preservation, directory type, competing-entry handling and cleanup. No suppressions were added. Fresh CI, Windows, CodeQL and Magi's review of the current commit remain merge gates.